src,lib: retrieve parsed source map url from v8 #44798

legendecas · 2022-09-26T16:56:28Z

V8 already parses the source map magic comments. Currently, only scripts
and functions expose the parsed source map URLs. It is unnecessary to
parse the source map magic comments again when the parsed information is
available.

Retrieving source map URL of a Module needs a V8 patch, working on
https://chromium-review.googlesource.com/c/v8/v8/+/3917379.

nodejs-github-bot · 2022-09-26T16:56:32Z

Review requested:

@nodejs/modules

V8 already parses the source map magic comments. Currently, only scripts and functions expose the parsed source map URLs. It is unnecessary to parse the source map magic comments again when the parsed information is available.

lib/internal/vm.js

joyeecheung · 2022-09-28T18:49:11Z

src/node_contextify.cc

+  if (result
+          ->Set(parsing_context,
+                env->source_map_url_string(),
+                fn->GetScriptOrigin().SourceMapUrl())


Does this work with the debug build? I think this being empty could trigger a DCHECK.

I added a test on no magic comment presents. Verified with debug build that no DCHECK are violated.

doc/api/vm.md

nodejs-github-bot · 2022-09-29T07:45:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46923/

jkrems · 2022-09-29T15:17:28Z

lib/internal/source_map/source_map_cache.js

-  // This is needed so that we don't match sourceMappingURL in string literals.
-  while ((match = RegExpPrototypeExec(kSourceMappingURLMagicComment, content))) {
-    lastMatch = match;
+  if (sourceMapURL === undefined) {


Is it by design that we still do the regex parse when V8 told us that there is no source map? If so, should we add a test case, e.g.:

checkSourceMapUrl(` function myFunc() {} ` //# sourceMappingURL=sourcemap.json `; `, 'sourcemap.json');

I think this fallback is kept here to handle the ES modules?

@jkrems @joyeecheung thanks for the suggestion. We don't need to apply the regex again when the parse result of the script indicates that no source mapping URL is available. I've updated the code to eliminate the duplicated scans.

It is true that the fallback is left here to handle the ES modules.

joyeecheung

LGTM

joyeecheung · 2022-09-30T12:14:29Z

lib/internal/source_map/source_map_cache.js

-  // This is needed so that we don't match sourceMappingURL in string literals.
-  while ((match = RegExpPrototypeExec(kSourceMappingURLMagicComment, content))) {
-    lastMatch = match;
+  if (sourceMapURL === undefined) {


I think this fallback is kept here to handle the ES modules?

doc/api/vm.md

nodejs-github-bot · 2022-10-05T08:28:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/47080/

lib/internal/vm.js

nodejs-github-bot · 2022-10-06T11:59:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/47099/

legendecas · 2022-10-11T16:45:54Z

@jkrems @joyeecheung would you mind taking a look again at this with the updates on #44798 (comment)? thank you!

legendecas · 2022-10-23T14:37:46Z

Landed in 6bdc101. Thank you for the reviews!

V8 already parses the source map magic comments. Currently, only scripts and functions expose the parsed source map URLs. It is unnecessary to parse the source map magic comments again when the parsed information is available. PR-URL: #44798 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 26, 2022

legendecas force-pushed the parse-sourcemap branch from a4ab0b5 to 936cc1e Compare September 26, 2022 16:58

src,lib: retrieve parsed source map url from v8

df4e7df

V8 already parses the source map magic comments. Currently, only scripts and functions expose the parsed source map URLs. It is unnecessary to parse the source map magic comments again when the parsed information is available.

legendecas force-pushed the parse-sourcemap branch from 936cc1e to df4e7df Compare September 26, 2022 17:02

legendecas added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. source maps Issues and PRs related to source map support. labels Sep 26, 2022

ljharb reviewed Sep 26, 2022

View reviewed changes

lib/internal/vm.js Outdated Show resolved Hide resolved

lib/internal/vm.js Outdated Show resolved Hide resolved

joyeecheung reviewed Sep 28, 2022

View reviewed changes

fixup! src,lib: retrieve parsed source map url from v8

86de757

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2022

jkrems reviewed Sep 29, 2022

View reviewed changes

github-actions bot mentioned this pull request Sep 30, 2022

CI Reliability 2022-09-30 nodejs/reliability#390

Open

17 tasks

joyeecheung approved these changes Sep 30, 2022

View reviewed changes

This was referenced Oct 1, 2022

CI Reliability 2022-10-01 nodejs/reliability#391

Open

CI Reliability 2022-10-02 nodejs/reliability#392

Open

jasnell reviewed Oct 2, 2022

View reviewed changes

doc/api/vm.md Outdated Show resolved Hide resolved

jasnell approved these changes Oct 2, 2022

View reviewed changes

github-actions bot mentioned this pull request Oct 3, 2022

CI Reliability 2022-10-03 nodejs/reliability#393

Open

9 tasks

fixup! src,lib: retrieve parsed source map url from v8

3566c6f

legendecas force-pushed the parse-sourcemap branch from 9c6839a to 3566c6f Compare October 5, 2022 07:48

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2022

anonrig reviewed Oct 5, 2022

View reviewed changes

lib/internal/vm.js Show resolved Hide resolved

anonrig approved these changes Oct 22, 2022

View reviewed changes

jkrems approved these changes Oct 23, 2022

View reviewed changes

legendecas closed this Oct 23, 2022

legendecas deleted the parse-sourcemap branch October 23, 2022 14:37

RafaelGSS mentioned this pull request Nov 1, 2022

v19.1.0 proposal #45269

Merged

danielleadams mentioned this pull request Dec 30, 2022

v18.13.0 release proposal #46025

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,lib: retrieve parsed source map url from v8 #44798

src,lib: retrieve parsed source map url from v8 #44798

legendecas commented Sep 26, 2022 •

edited

Loading

nodejs-github-bot commented Sep 26, 2022

joyeecheung Sep 28, 2022

legendecas Sep 29, 2022

nodejs-github-bot commented Sep 29, 2022

jkrems Sep 29, 2022

joyeecheung Sep 30, 2022 •

edited

Loading

legendecas Oct 5, 2022 •

edited

Loading

joyeecheung left a comment

joyeecheung Sep 30, 2022 •

edited

Loading

nodejs-github-bot commented Oct 5, 2022

nodejs-github-bot commented Oct 6, 2022

legendecas commented Oct 11, 2022

legendecas commented Oct 23, 2022

src,lib: retrieve parsed source map url from v8 #44798

src,lib: retrieve parsed source map url from v8 #44798

Conversation

legendecas commented Sep 26, 2022 • edited Loading

nodejs-github-bot commented Sep 26, 2022

joyeecheung Sep 28, 2022

Choose a reason for hiding this comment

legendecas Sep 29, 2022

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 29, 2022

jkrems Sep 29, 2022

Choose a reason for hiding this comment

joyeecheung Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

legendecas Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 5, 2022

nodejs-github-bot commented Oct 6, 2022

legendecas commented Oct 11, 2022

legendecas commented Oct 23, 2022

legendecas commented Sep 26, 2022 •

edited

Loading

joyeecheung Sep 30, 2022 •

edited

Loading

legendecas Oct 5, 2022 •

edited

Loading

joyeecheung Sep 30, 2022 •

edited

Loading